feat: improve Helm chart #12691

fernandezcuesta · 2025-06-25T11:41:32Z

Add extraInitContainers to celery+django deployments.
Add extraEnv to all deployments
Remove existing volume logic in favor of agnostic extraVolumes and extraVolumeMounts
Add the ability to deploy secrets as regular (non-hooked) resources due to issues found with ArgoCD
Make some secret references optional (secret might not be created)
Fix optional secret mounts + reference
Update bitnami chart reference (OCI)
Bump up redis chart

Reasoning behind the logic change for volumes, with the existing behaviour we cannot mount projected volumes (e.g. a secret mount where we want the key names being renamed) or per-container volumeMounts (e.g. nginx emptyDir when readOnlyRootFs is enforced).

- Add extraInitContainers to celery+django deployments. - Add extraEnv to all deployments - Remove existing volume logic in favor of agnostic extraVolumes and extraVolumeMounts - Fix optional secret mounts + reference - Update bitnami chart reference (OCI) - Bump up redis chart

dryrunsecurity · 2025-06-25T11:43:10Z

This pull request reveals multiple security vulnerabilities in the DefectDojo Helm chart, including optional critical security configurations, weak TLS settings for Redis, potential configuration injection risks, and the ability to override environment variables that could lead to unauthorized access or data compromise.

Missing or Optional Critical Configuration in helm/defectdojo/templates/tests/unit-tests.yaml

Vulnerability	Missing or Optional Critical Configuration
Description	The `DD_SECRET_KEY` and `DD_CREDENTIAL_AES_256_KEY` environment variables are made optional (`optional: true`) in the unit test pod and, critically, also in the main Django application deployment (as observed in hunk 37). These keys are fundamental for Django's security, including session management and data encryption. Making them optional means the application can run without these vital security components, leading to insecure session management, weakened data encryption, and potential data exposure. The unit tests, by mirroring this optionality, fail to detect this critical security misconfiguration, providing a false sense of security.

django-DefectDojo/helm/defectdojo/templates/tests/unit-tests.yaml

Lines 51 to 63 in 596b0c9

    
                     secretKeyRef: 
        
                       name: {{ $fullName }} 
        
                       key: DD_SECRET_KEY 
        
                       optional: true 
        
                 - name: DD_CREDENTIAL_AES_256_KEY 
        
                   valueFrom: 
        
                     secretKeyRef: 
        
                       name: {{ $fullName }} 
        
                       key: DD_CREDENTIAL_AES_256_KEY 
        
                       optional: true 
        
               resources: 
        
                 {{- toYaml .Values.tests.unitTests.resources | nindent 8 }} 
        
           restartPolicy: Never

Weak TLS Configuration by Default in helm/defectdojo/templates/configmap.yaml

Vulnerability	Weak TLS Configuration by Default
Description	The Helm chart for DefectDojo configures the Celery broker's connection to Redis. When Redis TLS is enabled (`.Values.redis.tls.enabled` is true) and the `redisParams` value is not explicitly overridden, the `DD_CELERY_BROKER_PARAMS` environment variable is set to `ssl_cert_reqs=optional`. This setting, as per Celery's documentation for Redis broker connections, means that the client will not validate the server's TLS certificate. This makes the connection vulnerable to Man-in-the-Middle (MitM) attacks, where an attacker could intercept or tamper with traffic between the application and the Redis broker without being detected by certificate validation.

django-DefectDojo/helm/defectdojo/templates/configmap.yaml

Lines 1 to 5 in 596b0c9

    
           {{- $fullName := include "defectdojo.fullname" . -}} 
        
           {{- $defaultBrokerParams := ternary "ssl_cert_reqs=optional" "" .Values.redis.tls.enabled -}} 
        
           apiVersion: v1 
        
           kind: ConfigMap 
        
           metadata:

Configuration Injection via extraConfigs in helm/defectdojo/templates/configmap.yaml

Vulnerability	Configuration Injection via extraConfigs
Description	The `helm/defectdojo/templates/configmap.yaml` template allows arbitrary key-value pairs from `.Values.extraConfigs` to be injected directly into the `data:` section of the ConfigMap. This block is placed at the very end of the `data:` section. Due to how YAML and Kubernetes ConfigMaps handle duplicate keys (last one wins), any key defined earlier in the ConfigMap can be overridden by a value provided in `.Values.extraConfigs`. This allows a user with control over Helm values to override critical security-related environment variables.

django-DefectDojo/helm/defectdojo/templates/configmap.yaml

Lines 55 to 60 in 596b0c9

    
           {{- if  .Values.django.uwsgi.certificates.enabled }} 
        
             REQUESTS_CA_BUNDLE: {{ .Values.django.uwsgi.certificates.certMountPath }}{{ .Values.django.uwsgi.certificates.certFileName }} 
        
           {{- end }} 
        
           {{- with .Values.extraConfigs }} 
        
             {{- toYaml . | nindent 2 }} 
        
           {{- end }}

Arbitrary Environment Variable Override in helm/defectdojo/templates/initializer-job.yaml

Vulnerability	Arbitrary Environment Variable Override
Description	The Helm chart templates for `initializer-job`, `celery-worker-deployment`, `celery-beat-deployment`, and `django-deployment` allow arbitrary environment variables to be injected via `*.extraEnv` fields. These `extraEnv` variables are processed after `envFrom` and other explicitly defined `env` variables. According to Kubernetes documentation, if a variable is defined in both `envFrom` (or `env`) and `env`, the `env` definition takes precedence. This means that a user with control over Helm values can override critical application settings, such as `DD_DATABASE_PASSWORD` (sourced from a secret), `DD_DATABASE_HOST`, or `DD_SECRET_KEY`, leading to potential misconfiguration, unauthorized access, or data compromise. An attacker could, for example, redirect the application to a malicious database or capture sensitive data by overriding the secret key.

django-DefectDojo/helm/defectdojo/templates/initializer-job.yaml

Lines 145 to 161 in 596b0c9

    
               name: {{ $fullName }} 
        
           - secretRef: 
        
               name: {{ $fullName }} 
        
               optional: true 
        
           - secretRef: 
        
               name: {{ $fullName }}-extrasecrets 
        
               optional: true 
        
           env: 
        
           - name: DD_DATABASE_PASSWORD 
        
             valueFrom: 
        
               secretKeyRef: 
        
                 name: {{ .Values.postgresql.auth.existingSecret }} 
        
                 key: {{ .Values.postgresql.auth.secretKeys.userPasswordKey }} 
        
           {{- with .Values.initializer.extraEnv }} 
        
           {{- toYaml . | nindent 8 }} 
        
           {{- end }} 
        
           resources:

Increased Attack Surface via Flexible Helm Values in helm/defectdojo/values.yaml

Vulnerability	Increased Attack Surface via Flexible Helm Values
Description	The Helm chart introduces highly flexible configuration options (`extraEnv`, `extraVolumes`, `extraInitContainers`, and configurable probes) that allow users to inject arbitrary environment variables, mount host paths, run additional init containers, and define custom probe commands. If an attacker gains control over the Helm `values.yaml` file or the Helm release, they could inject malicious configurations, leading to privilege escalation, sensitive data exfiltration, or container escape.

django-DefectDojo/helm/defectdojo/values.yaml

Lines 148 to 184 in 596b0c9

    
           # Components 
        
           celery: 
        
             broker: redis 
        
             logLevel: INFO 
        
             # Common annotations to worker and beat deployments and pods. 
        
             annotations: {} 
        
             beat: 
        
               # Annotations for the Celery beat deployment. 
        
               annotations: {} 
        
               affinity: {} 
        
               # Additional environment variables injected to Celery beat containers. 
        
               extraEnv: [] 
        
               # A list of additional initContainers to run before celery beat containers. 
        
               extraInitContainers: [] 
        
               # Array of additional volume mount points for the celery beat containers. 
        
               extraVolumeMounts: [] 
        
               # A list of extra volumes to mount 
        
               # @type: array<map> 
        
               extraVolumes: [] 
        
               # Enable liveness probe for Celery beat container. 
        
               livenessProbe: {} 
        
                 # exec: 
        
                 #   command: 
        
                 #     - bash 
        
                 #     - -c 
        
                 #     - celery -A dojo inspect ping -t 5 
        
                 # initialDelaySeconds: 30 
        
                 # periodSeconds: 60 
        
                 # timeoutSeconds: 10 
        
               nodeSelector: {} 
        
               # Annotations for the Celery beat pods. 
        
               podAnnotations: {} 
        
               # Enable readiness probe for Celery beat container. 
        
               readinessProbe: {} 
        
               replicas: 1 
        
               resources: 
        
                 requests:

All finding details can be found in the DryRun Security Dashboard.

github-actions · 2025-06-25T18:20:11Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

valentijnscholten · 2025-06-25T19:49:26Z

@fernandezcuesta Thanks for the PR, we'll ask some helm/k8s experienced devs to look at it. In the mean time could you look at the conflicts? Also this seems to be a bigger change, could you look at basing it against the dev branch

github-actions · 2025-06-26T06:28:52Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

fernandezcuesta · 2025-06-26T06:30:47Z

@valentijnscholten done, thanks for having a look at it! I also changed the base to dev branch.

kiblik

I will need more time for the rest (this is not my final review).

helm/defectdojo/Chart.lock

helm/defectdojo/Chart.yaml

helm/defectdojo/values.yaml

github-actions · 2025-08-21T10:56:44Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2025-08-21T11:51:24Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

fernandezcuesta · 2025-08-21T12:04:41Z

@kiblik Thanks for your detailed review and suggestions. I tried to go through all, but have some doubts about one of them (see inline comments).

kiblik

Many of the issues have been solved; some small ones are still open.
However, I'm still missing some info in the release notes
Mainly parts that have potential side-effects (rename annotations ot podAnnotations in celery)

helm/defectdojo/Chart.lock

helm/defectdojo/templates/configmap.yaml

kiblik · 2025-08-26T14:29:01Z

helm/defectdojo/values.yaml

-  # To use an external Redis instance, set enabled to false and uncomment
-  # the line below:
-  # redisServer: myrediscluster


Hmmm... I'm just checking _helpers.tpl and I'm thinking what is the right approach here 🤔.

django-DefectDojo/helm/defectdojo/templates/_helpers.tpl

Lines 45 to 67 in 941dd6c

{{/*

Determine the hostname to use for PostgreSQL/Redis.

*/}}

{{- define "postgresql.hostname" -}}

{{- if .Values.postgresql.enabled -}}

{{- if eq .Values.postgresql.architecture "replication" -}}

{{- printf "%s-%s-%s" .Release.Name "postgresql" .Values.postgresql.primary.name | trunc 63 | trimSuffix "-" -}}

{{- else -}}

{{- printf "%s-%s" .Release.Name "postgresql" | trunc 63 | trimSuffix "-" -}}

{{- end -}}

{{- else -}}

{{- printf "%s" ( .Values.postgresql.postgresServer | default "127.0.0.1" ) -}}

{{- end -}}

{{- end -}}

{{- define "redis.hostname" -}}

{{- if eq .Values.celery.broker "redis" -}}

{{- if .Values.redis.enabled -}}

{{- printf "%s-%s" .Release.Name "redis-master" | trunc 63 | trimSuffix "-" -}}

{{- else -}}

{{- printf "%s" (.Values.celery.brokerHost | default .Values.redis.redisServer) -}}

{{- end -}}

{{- end -}}

{{- end -}}

Postgres and Redis are both essential parts for DD.

They might be used as part of the stack, or both can be used aaS or any combination between

Postgres is used by all Django components (uwsgi, celery, initializer), same for Redis (well, except initializer, but definitely not only celery)

The following two statements should use the same conventions (they do not follow right now)

{{- printf "%s" ( .Values.postgresql.postgresServer | default "127.0.0.1" ) -}} {{- printf "%s" (.Values.celery.brokerHost | default .Values.redis.redisServer) -}}

helm/defectdojo/templates/tests/unit-tests.yaml

github-actions · 2025-08-27T07:13:30Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2025-08-27T07:18:35Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

kiblik

Just a small issue - I would drop the Bitnami reference from the release notes (see the context)

And TBH, I would probably change the chart version to 1.7.1 (it needs to be 1.7.1-dev, the suffix will be removed during releasing).
Because this is a radical change, and it should be noticeable in the version number as well. It not only adds value, but it might also affect existing deployments.

Huge thanks, this PR is a large cleanup and makes the chart more flexible and easier to read and use.

docs/content/en/open_source/upgrading/2.50.md

kiblik · 2025-08-27T11:44:33Z

helm/defectdojo/templates/_helpers.tpl

@@ -53,15 +53,16 @@ Create the name of the service account to use
 {{-   printf "%s-%s" .Release.Name "postgresql" | trunc 63 | trimSuffix "-" -}}
 {{-  end -}}
 {{- else -}}
-{{-  printf "%s" ( .Values.postgresql.postgresServer | default "127.0.0.1" ) -}}
+{{- .Values.postgresServer | default "127.0.0.1" | quote -}}


CC @lchastel
JFYI, your #12965 has not been released yet, but it will probably change the format before it is officially released. Please see #12691 (comment) for full context.

kiblik · 2025-08-27T11:48:28Z

helm/defectdojo/values.yaml

-  # To use an external Redis instance, set enabled to false and uncomment
-  # the line below:
-  # redisServer: myrediscluster


I really like this separation. I was planning to add values.schema.json (see #12984) to avoid wrong values. And I wasn't sure what to do with the overlap with subchart values. As soon as we separate them, it is much easier to adopt them.

Co-authored-by: kiblik <5609770+kiblik@users.noreply.github.com>

fernandezcuesta requested review from Maffooch and mtesauro as code owners June 25, 2025 11:41

github-actions bot added the helm label Jun 25, 2025

fernandezcuesta changed the title ~~**Summary:**~~ feat: improve Helm chart Jun 25, 2025

chore: add livenessProbe entries for celery

475ed17

Maffooch changed the base branch from master to bugfix June 25, 2025 17:47

github-actions bot added the conflicts-detected label Jun 25, 2025

fix: reference to removed field

6b18b96

fernandezcuesta changed the base branch from bugfix to dev June 26, 2025 06:28

fix: conflict

4165ca8

github-actions bot removed the conflicts-detected label Jun 26, 2025

fernandezcuesta added 4 commits June 26, 2025 08:32

chore: add reference to upstream chart

cbf8778

fix: missing default values from upstream chart used in templates

67d6348

chore: rephrase

b34e833

feat: allow deploy secret as regular non-hooked resources

330a127

Maffooch requested a review from rossops June 26, 2025 16:07

kiblik requested changes Jun 27, 2025

View reviewed changes

valentijnscholten added this to the 2.49.0 milestone Jun 29, 2025

fernandezcuesta added 3 commits June 30, 2025 09:19

fix: review

b474297

chore: restore Chart.lock

5c2f804

chore: update chart.lock

fba0cd0

rossops approved these changes Jul 7, 2025

View reviewed changes

fernandezcuesta requested a review from kiblik July 23, 2025 11:49

chore: wrap services url

108e7b4

github-actions bot added unittests ui parser labels Aug 21, 2025

Merge branch 'dev' into feat/improve-helm-chart

7bd3273

github-actions bot removed the conflicts-detected label Aug 21, 2025

github-actions bot added conflicts-detected and removed docker docs unittests ui parser labels Aug 21, 2025

fix: PR review suggestions

6794694

github-actions bot removed the conflicts-detected label Aug 21, 2025

chore: mount extraVolumes in initContainers too

3f502ad

fernandezcuesta requested a review from kiblik August 21, 2025 12:05

kiblik requested changes Aug 26, 2025

View reviewed changes

chore: move external db values to separate fields, add release notes

4f07742

github-actions bot added the docs label Aug 27, 2025

github-actions bot added the conflicts-detected label Aug 27, 2025

Merge branch 'dev' into feat/improve-helm-chart

77511d2

github-actions bot removed the conflicts-detected label Aug 27, 2025

kiblik requested changes Aug 27, 2025

View reviewed changes

fernandezcuesta and others added 2 commits August 28, 2025 07:15

Update docs/content/en/open_source/upgrading/2.50.md

25ebe6a

Co-authored-by: kiblik <5609770+kiblik@users.noreply.github.com>

chore: bump chart version and remove bitnami dependency comment from RN

596b0c9

kiblik approved these changes Aug 28, 2025

View reviewed changes

	{{/*
	Determine the hostname to use for PostgreSQL/Redis.
	*/}}
	{{- define "postgresql.hostname" -}}
	{{- if .Values.postgresql.enabled -}}
	{{- if eq .Values.postgresql.architecture "replication" -}}
	{{- printf "%s-%s-%s" .Release.Name "postgresql" .Values.postgresql.primary.name \| trunc 63 \| trimSuffix "-" -}}
	{{- else -}}
	{{- printf "%s-%s" .Release.Name "postgresql" \| trunc 63 \| trimSuffix "-" -}}
	{{- end -}}
	{{- else -}}
	{{- printf "%s" ( .Values.postgresql.postgresServer \| default "127.0.0.1" ) -}}
	{{- end -}}
	{{- end -}}
	{{- define "redis.hostname" -}}
	{{- if eq .Values.celery.broker "redis" -}}
	{{- if .Values.redis.enabled -}}
	{{- printf "%s-%s" .Release.Name "redis-master" \| trunc 63 \| trimSuffix "-" -}}
	{{- else -}}
	{{- printf "%s" (.Values.celery.brokerHost \| default .Values.redis.redisServer) -}}
	{{- end -}}
	{{- end -}}
	{{- end -}}

feat: improve Helm chart #12691

Are you sure you want to change the base?

feat: improve Helm chart #12691

Uh oh!

Conversation

fernandezcuesta commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dryrunsecurity bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 25, 2025

Uh oh!

valentijnscholten commented Jun 25, 2025

Uh oh!

github-actions bot commented Jun 26, 2025

Uh oh!

fernandezcuesta commented Jun 26, 2025

Uh oh!

kiblik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 21, 2025

Uh oh!

github-actions bot commented Aug 21, 2025

Uh oh!

fernandezcuesta commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiblik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kiblik Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 27, 2025

Uh oh!

github-actions bot commented Aug 27, 2025

Uh oh!

kiblik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kiblik Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

kiblik Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fernandezcuesta commented Jun 25, 2025 •

edited

Loading

dryrunsecurity bot commented Jun 25, 2025 •

edited

Loading

fernandezcuesta commented Aug 21, 2025 •

edited

Loading